Reimplement GWT assay plate designer in React by labkey-jeckels · Pull Request #7623 · LabKey/platform

labkey-jeckels · 2026-04-27T02:21:12Z

Rationale

Our assay plate designer is our final GWT UI. This replaces it with a React-based version.

Related Pull Requests

https://github.com/LabKey/limsModules/pull/2155

Changes

New React app that matches the GWT version 1:1
Improve generics in PlateManager and related callers

Tasks 📍

labkey-adam · 2026-04-27T17:02:42Z

👍

labkey-alan

I did not do the most thorough review on the React code, since I was only asked to take a cursory look at the code, and the GitHub outage is preventing me from viewing all of the files at this time. I do have two pieces of feedback though:

Many of the components have really large chunks of TSX code (e.g. GroupTypesPanel). Lots of nested loops and conditional statements. This is a readability mess, and it also makes it significantly harder to test, since we can't test in smaller units of functionality. My recommendation is to split these large chunks of TSX into smaller components.
There are zero unit tests. For a set of components this size, and this complex, we really should have as much unit test coverage as is reasonable.

Additionally I do have a concern about introducing this code more generally. While I understand the motivation to get rid of the last GWT component, and I would love to see us accomplish that goal, I am not sure that this is the best path forward. We already have a lot of plate related code in our apps, and while this code helps us get rid of GWT, it does not change the fact that we have two different implementations of a UI that is meant to accomplish the same thing. I think we would benefit most from having one UI for our plate code, not two different implementations that are both made with React.

I can take a more thorough look at the code later if wanted.

labkey-jeckels · 2026-04-28T02:12:24Z

---------------------------------------------|---------|----------|---------|---------|------------------------------------------------------------------------------------------------
File                                         | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                                                                              
---------------------------------------------|---------|----------|---------|---------|------------------------------------------------------------------------------------------------
All files                                    |   73.37 |    74.48 |   68.29 |   76.86 |                                                                                                
 src/client/PlateTemplateDesigner            |   47.63 |    39.39 |   29.57 |   53.49 |                                                                                                
  PlateTemplateDesigner.tsx                  |   43.22 |    33.33 |   28.57 |   48.86 | ...283,287-289,293-301,305-319,324,328-329,339-346,352-365,370-377,382,386-387,393-395,416-418 
  models.ts                                  |     100 |      100 |     100 |     100 |                                                                                                
 src/client/PlateTemplateDesigner/components |   98.35 |    96.65 |   97.84 |    99.2 |                                                                                                
  GroupTypesPanel.tsx                        |   96.62 |    95.18 |   94.28 |   97.29 | 280,329                                                                                        
  MultiCreateDialog.tsx                      |   98.36 |    94.11 |     100 |     100 | 24,35                                                                                          
  RightPanel.tsx                             |     100 |      100 |     100 |     100 |                                                                                                
  ShiftPanel.tsx                             |     100 |      100 |     100 |     100 |                                                                                                
  StatusBar.tsx                              |     100 |      100 |     100 |     100 |                                                                                                
  TemplateGrid.tsx                           |     100 |      100 |     100 |     100 |                                                                                                
  WarningPanel.tsx                           |     100 |      100 |     100 |     100 |                                                                                                
  WellGroupProperties.tsx                    |   95.83 |    92.85 |     100 |     100 | 46                                                                                             
 test/js                                     |     100 |      100 |     100 |     100 |                                                                                                
  fileMock.js                                |     100 |      100 |     100 |     100 |                                                                                                
---------------------------------------------|---------|----------|---------|---------|------------------------------------------------------------------------------------------------

labkey-jeckels · 2026-04-29T00:39:53Z

I did not do the most thorough review on the React code, since I was only asked to take a cursory look at the code, and the GitHub outage is preventing me from viewing all of the files at this time. I do have two pieces of feedback though:

Many of the components have really large chunks of TSX code (e.g. GroupTypesPanel). Lots of nested loops and conditional statements. This is a readability mess, and it also makes it significantly harder to test, since we can't test in smaller units of functionality. My recommendation is to split these large chunks of TSX into smaller components.

There are zero unit tests. For a set of components this size, and this complex, we really should have as much unit test coverage as is reasonable.

Additionally I do have a concern about introducing this code more generally. While I understand the motivation to get rid of the last GWT component, and I would love to see us accomplish that goal, I am not sure that this is the best path forward. We already have a lot of plate related code in our apps, and while this code helps us get rid of GWT, it does not change the fact that we have two different implementations of a UI that is meant to accomplish the same thing. I think we would benefit most from having one UI for our plate code, not two different implementations that are both made with React.

I can take a more thorough look at the code later if wanted.

Refactored. See what you think now.
Significant unit testing added, along with endpoint testing (with help from Nick).

labkey-tchad · 2026-04-29T16:03:47Z

Manual testing looks mostly good. Just some problems with the buttons at the top.

The "Save & Close" and "Cancel" buttons trigger a dirty page alert if there are changes.
The "Save & Close" button is enabled when there aren't any changes
None of the buttons match existing LabKey button styling

labkey-tchad · 2026-04-29T23:26:36Z

I just noticed a functionality downgrade in the new component.
There isn't any visual connection between the plate grid and the list of well groups other than their color. This is passable for a small number of groups on a smaller plate but it is pretty unintelligible on a large plate with a lot of groups (such as the non-empty 384 well NAb templates).

In the GWT designer, we display the well and group names next to the cancel button when you mouse over a well:

The GWT designer also highlights corresponding plate grid gutters when you mouse over a group:

The new designer already applies a class (template-grid__cell--active) to any wells associated with the currently selected group so it would just require us to assign some styling to that class for the plate grid to highlight the currently selected group.

labkey-jeckels · 2026-04-30T00:53:11Z

Manual testing looks mostly good. Just some problems with the buttons at the top.

The "Save & Close" and "Cancel" buttons trigger a dirty page alert if there are changes.

The "Save & Close" button is enabled when there aren't any changes

None of the buttons match existing LabKey button styling

Save & Close has typically been enabled even when the page isn't dirty. That's retained from GWT.

Other button issues should be fixed - I matched the normal SDMS styling.

labkey-jeckels · 2026-04-30T00:53:47Z

I just noticed a functionality downgrade in the new component. There isn't any visual connection between the plate grid and the list of well groups other than their color. This is passable for a small number of groups on a smaller plate but it is pretty unintelligible on a large plate with a lot of groups (such as the non-empty 384 well NAb templates).

In the GWT designer, we display the well and group names next to the cancel button when you mouse over a well:

The GWT designer also highlights corresponding plate grid gutters when you mouse over a group:

The new designer already applies a class (template-grid__cell--active) to any wells associated with the currently selected group so it would just require us to assign some styling to that class for the plate grid to highlight the currently selected group.

I didn't bother with the gutter highlighting, but there's now feedback between the selected group and its wells and vice versa.

labkey-tchad · 2026-04-30T19:53:22Z

The button behavior doesn't seem to have changed at all. The "Save & Close" and "Cancel" buttons still alert you about unsaved changes.
Whether or not a "Cancel" button should alert you about unsaved changes is arguable, I suppose, but "Save & Close" definitely shouldn't.

labkey-jeckels · 2026-05-02T20:40:28Z

The button behavior doesn't seem to have changed at all. The "Save & Close" and "Cancel" buttons still alert you about unsaved changes. Whether or not a "Cancel" button should alert you about unsaved changes is arguable, I suppose, but "Save & Close" definitely shouldn't.

Strange, I don't know why I couldn't repro the Save & Close before. I removed the prompt from both buttons. The GWT designer confirmed on Cancel from a dirty page, but from spot checking our other React UIs we aren't doing it elsewhere.

labkey-alan

I've left some feedback. Overall it looks pretty good, but there are several changes that I think we should make.

One general piece of feedback is that function components should be typed as FC, and they should all have their displayName set. Most (all?) components are defined as function MyComponent(props: MyPropsType): JSX.Element, which is not a pattern we use at LabKey, and I think is worse than the const MyComponent: FC<MyPropsType> = () => {} pattern.

Another piece of feedback is that there is very little consistency in how we handle Events. Sometimes useCallback is used, but even then sometimes it's used in a way that doesn't matter (it's wrapped in a function that is regenerated on every render). Other times we create named functions for event handlers, but they're not memoized with useCallback. Often we just have inline event handlers. While techincally speaking you don't need to memoize callbacks handed to native DOM elements, we tend to do that at LabKey. The consistency is nice, but also because refactors or new features happen reasonably often it tends to be safer to memoize it today, because tomorrow's usage may benefit from it. There's very little overhead to useCallback, and we've seen real performance issues from omitting it.

labkey-alan · 2026-05-04T14:48:25Z

+                        panelId={`group-panel-${type}`}
+                        isActive={type === activeTab}
+                        baseClass="group-types-panel__tab"
+                        onClick={() => onTabChange(type)}


The TabButton component here will re-render on every render cycle because we are always passing a fresh onClick function to it. Instead, you should pass onTabChange and type as props, then in TabButton you should define onClick as const onClick = useCallback(() => onTabChange(type), [onTabChange, type]). Then, React can skip rendering these TabButton components unless onTabChange or type changes, which is unlikely.

labkey-alan · 2026-05-04T14:51:47Z

+                                    const isHighlighted = hoveredWellGroupId === group.rowId;
+                                    const isRenaming = renamingId === group.rowId;
+                                    return (
+                                        <React.Fragment key={group.rowId}>


This whole React.Fragment smells like a component to me.

Extracted but it doesn't feel great due to the large number of properties. I did some reduction but open to further ideas.

I agree that it's not the most ideal because of the large number of props, I do think it is still more readable to separate it into a separate component. Here are a few suggestions for simplifying it further:

Drop color and colorIndex props, and just pass the colorMap as one prop, do the same calculations in GroupRow

Make the GroupRow component handle the events (e.g. event.stopPropagation) instead of the parent

Move state (isRenaming, isConfirmingDelete) into the GroupRow component

This would remove the need for these props, and would let you drop renamingId and confirmDeleteId from GroupTypesPanel

It's possible this would change behavior in a way that we don't want, by moving the state into the GroupRow you'd be able to click delete on multiple rows and have them all showing the confirmation buttons at the same time. I think that's fine, but maybe that's a change in behavior we don't want to make.

If we don't want to change the behavior, we could still reduce the number of props by just passing setRenamingId and setConfirmDeleteId as a props instead. This would let us pass setConfirmDeleteId instead of handleDeleteConfirm and handleDeleteCancel, reducing the number of props needed (and do the equivalent for renamingId).

I'll add a suggested change in my current review.

Suggested change was too big to put in a review so I sent you a patch via chat.

labkey-alan · 2026-05-04T17:16:17Z

+
+/*
+ * ──────────────────────────────────────────────────────────────────────────────
+ * Overall layout (vertical stack):


This comment is unnecessary.

FWIW, I asked Claude to generate these and other comments to help me understand the structure. I'm OK to delete or simplify them if they're not useful to others. If you have a pointer to an example that's been useful in other code, I'd be happy to adapt to it.

I don't have a pointer to more useful comments in our CSS, but to me a comment like this, which is a crude ASCII mockup of the UI is not particularly useful, when a develop could just open the app in question and see the UI, it's also not a comment that would be easy to maintain.

…nel.test.tsx Co-authored-by: Alan Vezina <alanv@labkey.com>

Co-authored-by: Alan Vezina <alanv@labkey.com>

….tsx Co-authored-by: Alan Vezina <alanv@labkey.com>

labkey-alan · 2026-05-05T17:15:13Z

+    // row and col are stable for a given cell instance (position never changes), so these
+    // callbacks remain stable as long as the parent handlers are stable useCallback refs.
+    const handleMouseDown  = useCallback((e: React.MouseEvent)    => onMouseDown(row, col, e),  [onMouseDown, row, col]);
+    const handleMouseEnter = useCallback(()                        => onMouseEnter(row, col),   [onMouseEnter, row, col]);


This formatting is incredibly weird. I will push a change that adds our eslint-config as a dependency. You'll want to run the npm run lint-fix command after I push, and commit the changes.

labkey-alan

Overall it looks good. I have a few minor pieces of feedback, but nothing that really needs to block merging

We typically import FC and other things from React via import React, { FC, useCallback } from 'react'; instead of import React from 'react' and using React.FC. If you see that type of code in our codebase it's probably very old. This is mostly an aesthetics thing, and not a big deal.
A lot of the code is formatted oddly, some more egregiously than others. I've pushed a commit to add our linter config to the assay package, you should run npm run lint-fix and commit the changes sometime before merging (and really any time after you make changes).
I do think we should update GroupRow to handle all events (e.g. e.stopPropagation) in the component and not the parent. I sent you a patch with these changes and more.

labkey-jeckels · 2026-05-06T01:26:28Z

Overall it looks good. I have a few minor pieces of feedback, but nothing that really needs to block merging

We typically import FC and other things from React via import React, { FC, useCallback } from 'react'; instead of import React from 'react' and using React.FC. If you see that type of code in our codebase it's probably very old. This is mostly an aesthetics thing, and not a big deal.

A lot of the code is formatted oddly, some more egregiously than others. I've pushed a commit to add our linter config to the assay package, you should run npm run lint-fix and commit the changes sometime before merging (and really any time after you make changes).

I do think we should update GroupRow to handle all events (e.g. e.stopPropagation) in the component and not the parent. I sent you a patch with these changes and more.

Updated. I didn't touch other references like React.ReactNode since other code seems to indicate that's OK.
Ran with some difficulties. I cleared a handful of errors and warnings from the linter.
Patch adopted. Had to move group from SharedProps to GroupRowProps and then discovered that this broke renaming a group. Fixed that and added unit test coverage.

I think this is finalized. @labkey-tchad there's been a fair bit of churn since you tested. Might be worth a quick sanity check. From my own testing and automated tests, things seem good.

labkey-jeckels added 5 commits April 24, 2026 20:49

Switch from GWT to React

b0ca7b8

Auto code review and comments

ca99f5e

Test fixes

a4b284f

Other cleanup

0622405

Improve generics for plate code

19e61e8

labkey-jeckels self-assigned this Apr 27, 2026

This was referenced Apr 27, 2026

Update test code to match React-based plate designer LabKey/testAutomation#2967

Open

Improve generics for plate code LabKey/commonAssays#1007

Open

labkey-nicka requested review from labkey-klum and labkey-nicka April 27, 2026 17:30

labkey-jeckels requested a review from labkey-alan April 27, 2026 17:46

labkey-alan requested changes Apr 27, 2026

View reviewed changes

labkey-jeckels added 2 commits April 27, 2026 19:08

Merge branch 'refs/heads/develop' into fb_reactPlateDesigner

136e9f8

Unit tests and other improvements

a912417

labkey-jeckels mentioned this pull request Apr 28, 2026

Add exceptions for unit tests and JSX structure LabKey/server#1353

Open

Another checkpoint

6851d9c

labkey-nicka reviewed Apr 28, 2026

View reviewed changes

Comment thread assay/src/org/labkey/assay/PlateController.java Outdated

Comment thread assay/src/org/labkey/assay/PlateController.java Outdated

Comment thread assay/src/org/labkey/assay/PlateController.java Outdated

Comment thread assay/src/org/labkey/assay/PlateController.java Outdated

labkey-jeckels added 2 commits April 28, 2026 17:28

Merge branch 'develop' into fb_reactPlateDesigner

6eea87a

Code review improvements, generics updates

28daef7

Unit test fixup, bounds validation

82f1f83

labkey-nicka approved these changes Apr 29, 2026

View reviewed changes

Comment thread assay/src/org/labkey/assay/PlateController.java Outdated

Comment thread assay/src/org/labkey/assay/PlateController.java Outdated

Merge branch 'develop' into fb_reactPlateDesigner

507c281

Manual test and code review feedback

3cdf006

Don't warn on intentional navigation away from a dirty page

72b7ae4

labkey-alan requested changes May 4, 2026

View reviewed changes

labkey-jeckels and others added 6 commits May 4, 2026 15:42

Update assay/src/client/PlateTemplateDesigner/components/GroupTypesPa…

70c1908

…nel.test.tsx Co-authored-by: Alan Vezina <alanv@labkey.com>

Update assay/src/client/PlateTemplateDesigner/components/ShiftPanel.tsx

37b97d4

Co-authored-by: Alan Vezina <alanv@labkey.com>

Update assay/src/client/PlateTemplateDesigner/components/WarningPanel…

a346db8

….tsx Co-authored-by: Alan Vezina <alanv@labkey.com>

Merge branch 'refs/heads/develop' into fb_reactPlateDesigner

f3374d6

Checkpoint

861dbc9

Address code review suggestions. A little more accessibilty improvement

03b3dec

labkey-jeckels requested a review from labkey-alan May 5, 2026 16:39

labkey-alan reviewed May 5, 2026

View reviewed changes

Add eslint-config dependency to assay package

1e4666f

labkey-alan approved these changes May 5, 2026

View reviewed changes

labkey-jeckels added 3 commits May 5, 2026 17:32

More code review

501921a

Fix renames

be56aed

eslint

5651c20

Merge branch 'develop' into fb_reactPlateDesigner

4fe6c32

Conversation

labkey-jeckels commented Apr 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale

Related Pull Requests

Changes

Tasks 📍

Uh oh!

labkey-adam commented Apr 27, 2026

Uh oh!

labkey-alan left a comment

Choose a reason for hiding this comment

Uh oh!

labkey-jeckels commented Apr 28, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

labkey-jeckels commented Apr 29, 2026

Uh oh!

Uh oh!

Uh oh!

labkey-tchad commented Apr 29, 2026

Uh oh!

labkey-tchad commented Apr 29, 2026

Uh oh!

labkey-jeckels commented Apr 30, 2026

Uh oh!

labkey-jeckels commented Apr 30, 2026

Uh oh!

labkey-tchad commented Apr 30, 2026

Uh oh!

labkey-jeckels commented May 2, 2026

Uh oh!

labkey-alan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

labkey-jeckels May 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

labkey-alan May 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

labkey-alan left a comment

Choose a reason for hiding this comment

Uh oh!

labkey-jeckels commented May 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

labkey-jeckels commented Apr 27, 2026 •

edited

Loading

labkey-jeckels May 5, 2026 •

edited

Loading

labkey-alan May 5, 2026 •

edited

Loading